Conversation
bartgol
added
cmake
There was a problem hiding this comment.
Pull request overview
This PR fixes how EKAT decides whether to fetch/build certain third-party
libraries (TPLs) by standardizing the decision logic on <pkg>_FOUND, and it
ensures yaml-cpp::yaml-cpp is available to downstream consumers via EKAT’s
installed CMake config.
Changes:
- Switch Catch2/yaml-cpp/spdlog FetchContent decisions to use
<pkg>_FOUND. - Ensure
<pkg>_FOUNDis set whenEKAT_SKIP_FIND_<PKG>is enabled. - Add downstream
yaml-cpp::yaml-cppalias creation inEkatConfig.cmake.in.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/testing-support/CMakeLists.txt | Uses Catch2_FOUND to decide whether to FetchContent Catch2. |
| src/parser/CMakeLists.txt | Uses yaml-cpp_FOUND to decide whether to FetchContent yaml-cpp. |
| src/logging/CMakeLists.txt | Uses spdlog_FOUND to decide whether to FetchContent spdlog. |
| cmake/EkatConfig.cmake.in | Ensures yaml-cpp::yaml-cpp exists for downstream consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The last version should be extra-extra safe:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If we found yaml-cpp (via find_package or as an existing tgt) | ||
| # ensure it provides the scoped target | ||
| if (yaml-cpp_FOUND) | ||
| if (TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp) | ||
| add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp) | ||
| endif() |
There was a problem hiding this comment.
yaml-cpp_FOUND can be TRUE even if neither yaml-cpp::yaml-cpp nor yaml-cpp targets exist (some installs set *_FOUND but don't export targets). In that case, the build path is skipped but ekat_yamlparser still links to yaml-cpp::yaml-cpp, which will fail at configure/generate time. Please add an explicit check: if yaml-cpp_FOUND is TRUE and the targets are missing, either create the required imported target or raise a FATAL_ERROR (or set yaml-cpp_FOUND back to FALSE to fall back to FetchContent).
There was a problem hiding this comment.
This is starting to look a lot like having to write our own FindYamlCpp.cmake... Gemini seems to agree with me:
Copilot is technically correct, but it’s suffering from "Perfectionist's Bloat."
In the world of CMake, there is a point of diminishing returns where you transition from "robust build system" to "writing a full-blown package manager." Copilot wants you to reach the latter.
The current impl would work for 99.99% of the cases. I think we're safe enough for now. If someone loses an hour of work b/c this issue fires back, I'll gladly buy them a few rounds of drinks to make up for it.
bartgol
force-pushed
the
bartgol/standardize-tpl-found-logic
branch
2 times, most recently
from
7d02f32 to
2fbec94
Compare
bartgol
force-pushed
the
bartgol/standardize-tpl-found-logic
branch
from
2fbec94 to
9b088bb
Compare
Motivation
As shown in the failed CI runs for
eamxx-v1workflow in E3SM-Project/E3SM#8258, there is an issue with how we decide whether to fetch and install yaml-cpp. Namely, if the pkg is found, and only provides the (nicely) scopedyaml-cpp::yaml-cpptarget, the if condition that establish whether to call FetchContent is wrong. This PR changes it to be more robust, for all the three packages: ensure that _FOUND is always set, and use that to decide whether to use FetchContent.Side fix: if the yaml-cpp pkg is found but does not define the scoped tgt, we create the scoped one ourselves. But we were not propagating this to the installed EkatConfig.cmake, so downstream tgt would not know what yaml-cpp::yaml-cpp was (again, only for the case where a pre-installed yaml-cpp did NOT define the scoped tgt). To fix this, I added the same logic to our EkatConfig.cmake.in file, so that the scoped target is guaranteed to exist for downstream customers.
E3SM Stakeholder Feedback
Testing